Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

環境変数 SKIP_CREATE_GITHASH が 1 にセットしている場合、githash.h の生成をスキップする #319

Conversation

m-tmatma
Copy link
Member

@m-tmatma m-tmatma commented Aug 2, 2018

環境変数 SKIP_CREATE_GITHASH が 1 にセットしていてかつ githash.h が存在しない場合
githash.h の生成をスキップできるようにする。

何らかのリファクタリングを行ったあと、コード (asm) あるいはリソース (res ファイル)が
完全一致するか確認するために使用する。

通常ではコミットハッシュが異なると、githash.h の内容が異なるのでバイナリが一致しない。
リファクタリングしたときに生成コードとres ファイルの完全一致を確認できると確認がしやすい。

※ ローカルビルド用の機能です。
※ 別途ビルド関連の説明を行う markdown を更新する PR を投げる予定です。

テスト方法

  • build-sln.bat Win32 Release
    • → githash.h が更新される
  • set SKIP_CREATE_GITHASH=1
  • build-sln.bat Win32 Release
    • → githash.h が更新されない
  • set SKIP_CREATE_GITHASH=0
  • build-sln.bat Win32 Release
    • → githash.h が更新される

@m-tmatma m-tmatma added the enhancement ■機能追加 label Aug 2, 2018
@m-tmatma m-tmatma added this to the next release milestone Aug 2, 2018
@m-tmatma
Copy link
Member Author

m-tmatma commented Aug 2, 2018

テスト方法の追加

  • githash.h を削除しておく
  • set SKIP_CREATE_GITHASH=1
  • build-sln.bat Win32 Release
    → githash.h が作成される

@berryzplus
Copy link
Contributor

リファクタリングとは、振る舞いを変えずにコードを変更することだと思っています。
コードを変えたらasmは変わりますし、当然オブジェクトのハッシュも変更になります。
hash値は、コミットのたびに変わるのが自然だと思います。

変わらないように細工できるメリットがよくわかっていません。
そのあたりが理解できてないと本質的なツッコミを入れられないので、
漫然とコードを眺めて主題と関係ないところにいちゃもんつける結果になりそうです。

まずは、これができると何がうれしいのか、についての説明をお願いします。

@m-tmatma
Copy link
Member Author

m-tmatma commented Aug 3, 2018

文字コード変換のときや、リソースファイルで複数行に分かれているときなどに
asm, res ファイルが完全一致するのを確認することが何度かあったので、
次回同じようなことがあったときに確認しやすいように対応しました。

@berryzplus
Copy link
Contributor

微妙だと感じました。

文字コード変換のときや、リソースファイルで複数行に分かれているときなどに
asm, res ファイルが完全一致するのを確認することが何度かあったので、
次回同じようなことがあったときに確認しやすいように対応しました。

通常、ソースコードの変更は不具合の修正や機能追加で行われるものです。
変更の結果何も変わらないなら、そもそも変更する必要があるんでしょうか?
完全一致で確認できる変更は、例外中の例外だと考えています。
基本的に「完全一致で確認できる変更はない」と思っていいと思います。

というか、githash が変更されると困る理由を突き詰めて考えてみると、「githashを変更しないオプションを追加する」という対応は要らない気がします。

githashが変更されると困る
 何故? → githashが変更されると CDlgAbout.asm に差異が出る(推定)
 何故? → CDlgAbout.cppにgithashの文字列が埋め込まれている(事実)
 何故? → svnrev.hのときからそうだったから(事実)
 何故? → svnrevは番号なのでリソース化する必要はないと思ったから(推定)

問題の本質は、本来リソースに格納すべき情報をcppに直接埋め込んでいることにあると思います。
svnrev.hのときにどういう判断で埋め込みを選択したのか分かりませんが、処理と関係ないリソースはコードと切り離して格納すべきです。windows API プログラムの場合、切り離し先はリソーススクリプトになります。githashの参照をリソースに追い出したら、githashが変更されても困らなくなりますよね?そうするとgithashを変更しないオプションは要らないことになるわけです。


asmの完全一致で確認できる変更は今後も結構あると思っています。

  • コメントの誤り訂正
  • 引数名を宣言に含めていない関数定義の修正
  • #ifディレクティブによりビルドされない無効ブロックの除去
  • 不要な#define(古い Windows SDKのための定義)の除去
  • 冗長な独自マクロの除去
  • SALアノテーションの追加

これらの対応を計画していて「~のために必要」というのは理解できます。
githashの変更がasm比較に影響を与えない工夫が必要であることと、githashを変更しないオプションを追加する対応は別の話だと思っています。

@m-tmatma
Copy link
Member Author

m-tmatma commented Aug 5, 2018

問題の本質は、本来リソースに格納すべき情報をcppに直接埋め込んでいることにあると思います。
svnrev.hのときにどういう判断で埋め込みを選択したのか分かりませんが、処理と関係ないリソースはコードと切り離して格納すべきです。windows API プログラムの場合、切り離し先はリソーススクリプトになります。githashの参照をリソースに追い出したら、githashが変更されても困らなくなりますよね?そうするとgithashを変更しないオプションは要らないことになるわけです。

#320 等リソースファイルの変更で全くバイナリが変わっていないことを確認するために
何度も githash.bat の処理が実行されないようにローカルで小細工する処理は何度も
行っています。

cpp の検証だけに使うのではありません。

@berryzplus
Copy link
Contributor

#320, リソースファイルのリファクタリング (複数行にわかれているのを一行にまとめる)の対応漏れの対応 等リソースファイルの変更で全くバイナリが変わっていないことを確認するために何度も githash.bat の処理が実行されないようにローカルで小細工する処理は何度も行っています。

OK、ようやく言ってる意味が分かってきました。

ぼくは cpp→obj の変換で、内容を何も変えなくてもバイナリが異なることからオブジェクトファイルにはタイムスタンプ情報が埋め込まれるものだと思っていました。

rc → res の変換については今まで確認を行ったことはありませんでした。「同じ」オブジェクトファイルなんだからタイムスタンプが埋め込まれて、毎回意味の分からない差異が生まれるんだろうと思っていました。

「resは違う」って言ってますよね?
リソースそのものに差異がなければ、バイナリファイルが完全一致します。
何度も確認してます、と。
タイムスタンプどころか、行番号すら含まれていないはずだ、と。

そこがうまく伝わっていませんでした。
どこかで書きましたが、ぼくは githash の内容をリソースに埋め込むべきだと考えています。
処理に関係のない情報なので、cppやcppが使うヘッダからは分離したほうがいいように思うので。
rc → res が リソースの内容変更以外で変更されない変換であるならば、
githashの更新を止めるのは有効だと思います。

・・・という前提で再レビュー行ってきます。

berryzplus
berryzplus previously approved these changes Aug 5, 2018
Copy link
Contributor

@berryzplus berryzplus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTMです。

githash生成をスキップする手順は、そのうちどこかにまとまるもんだと思ってます。

@@ -88,6 +88,20 @@ if "%APPVEYOR_BUILD_URL_VALID%" == "1" (
set GITHASH_H=%OUT_DIR%\githash.h
set GITHASH_H_TMP=%GITHASH_H%.tmp

@rem check if skip creation of %GITHASH_H%
set VALID_CREATE_GITHASH=1
if "%SKIP_CREATE_GITHASH%" == "1" (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rem set SKIP_CREATE_GITHASH=1

ってあった方がいいような・・・。

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

説明を足しました。

@Rem set SKIP_CREATE_GITHASH=1

バッチファイルは変えない前提です。
バッチファイルを変えるのだったらこの PR は必要ないです。

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

理解しました。OKです。

@m-tmatma
Copy link
Member Author

m-tmatma commented Aug 5, 2018

githash生成をスキップする手順は、そのうちどこかにまとまるもんだと思ってます。

後でやろうと思ってしましたが、
README.md に追加しました。

Copy link
Contributor

@berryzplus berryzplus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

対応ありがとうございます。再LGTMです。

@@ -88,6 +88,20 @@ if "%APPVEYOR_BUILD_URL_VALID%" == "1" (
set GITHASH_H=%OUT_DIR%\githash.h
set GITHASH_H_TMP=%GITHASH_H%.tmp

@rem check if skip creation of %GITHASH_H%
set VALID_CREATE_GITHASH=1
if "%SKIP_CREATE_GITHASH%" == "1" (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

理解しました。OKです。

@berryzplus
Copy link
Contributor

「再」じゃないね 😄

@m-tmatma m-tmatma merged commit 96ab230 into sakura-editor:master Aug 5, 2018
@m-tmatma m-tmatma deleted the feature/skip-create-githash-if-not-exists branch August 5, 2018 21:30
@kobake
Copy link
Member

kobake commented Aug 12, 2018

@m-tmatma
すみません、遅レスですけど、README.md に記述する内容は「多くの人にとって必要なもの」に絞りたいです。

今回の githash.h のスキップ機能は必要な機能ではあるのですが、このスキップ機能を意識するのはごく少数のプログラマだけのはずです。そういった細かい情報はできれば Wiki のほうに書いたほうが README.md がスッキリして良いと思うのですがどうでしょうか。

@m-tmatma m-tmatma mentioned this pull request Aug 12, 2018
@m-tmatma
Copy link
Member Author

#341 で更新しました。

@ds14050 ds14050 added the enhancement ■機能追加 label Sep 18, 2018
HoppingTappy pushed a commit to HoppingTappy/sakura that referenced this pull request Jun 11, 2019
…e-githash-if-not-exists

環境変数 SKIP_CREATE_GITHASH が 1 にセットしている場合、githash.h の生成をスキップする
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement ■機能追加
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants